fix(security): stop a project's CSRF policy blocking its own release asset build - #3641
Conversation
…asset build
A release asset manifest is built by the project runtime, not the CLI: the
control plane POSTs a signed operation envelope to
`/api/control-plane/runs/{runId}/execute` with `target:
"task:release-asset-build"`, and only that dispatch calls
`beginReleaseAssetManifestBuild`. `CsrfHandler` runs at priority 5 with an
empty pattern list, so it sits in front of that POST, and the control plane
holds no `__Host-vf_csrf` cookie to echo -- it authorizes with a signature the
receiving handler verifies. Any project that set `security.csrf` to anything
truthy therefore answered its own build dispatch with
`403 Forbidden - invalid or missing CSRF token`.
Nothing downstream could see it. The run failed before the manifest row
existed, so the state stayed `missing` and `veryfront deploy` reported
`Release assets were not ready within 120s (last state: missing)` -- naming
neither CSRF nor config. A customer demo lost the safe option
(`csrf: { excludePaths: ["/api/ag-ui"] }`) and shipped with CSRF off entirely.
The report attributed this to the nested object shape; the boundary is
narrower and worse than that: `csrf: true` fails identically, and only an
absent or `false` setting ever built a manifest.
`isControlPlaneSurfaceRequest` exempts the signed control-plane prefix, the
same shape as the existing `isCspReportRequest` exemption and for the same
reason: the gate expects a browser credential the caller cannot hold, and
every handler behind that prefix authenticates its envelope through
`verifyControlPlaneRequest` before acting. The prefix is matched against
`URL.pathname`, which resolves dot segments, and a path that merely starts
alike (`/api/control-plane-mirror/...`) stays gated.
Covered by a dispatch test that drives the real chain -- `CsrfHandler` then
`ProjectRunExecuteHandler`, with a signed envelope -- across all four csrf
shapes and asserts the release asset build executor is reached.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds shared control-plane route classification and signed dispatch detection. The CSRF handler bypasses validation for registered signed dispatches. Tests cover route rejection and release-asset manifest execution across CSRF configurations. ChangesControl-plane CSRF dispatch
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ControlPlaneRequest
participant CsrfHandler
participant ProjectRunExecuteHandler
participant ManifestBuilder
ControlPlaneRequest->>CsrfHandler: Submit signed execution request
CsrfHandler->>ProjectRunExecuteHandler: Continue without CSRF token validation
ProjectRunExecuteHandler->>ManifestBuilder: Begin manifest building
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e055307f43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…patch The exemption matched on path prefix alone, so any request under `/api/control-plane/` skipped the CSRF gate. That namespace is reserved but not exclusively routed: only five method/path shapes reach a handler that verifies a signed envelope, and everything else under the prefix falls through to `ApiHandlerWrapper`. A project App or Pages API route placed under the prefix in a custom runtime is cookie authenticated, so the prefix match let a project turn CSRF off on its own state-changing routes by choosing a path. `isSignedControlPlaneDispatch` now requires both conditions: the method and path must address a registered control-plane surface, and the request must carry the control-plane signature header the receiving handler verifies. The signature covers the request method and path, so an envelope cannot be replayed against another surface, and a browser cannot attach that header cross-origin without a preflight the runtime does not grant. The route table moves into `channels/control-plane.ts` as `isControlPlaneSurfaceRoute`, and the proxy classifier consumes it instead of keeping a second copy of the same patterns. Tests cover a project route inside the namespace, a signature header on an unrecognized path, and a registered surface with no signature: all three still enforce CSRF. Also drops a customer hostname from the regression test in favour of `example.test`, and removes em dashes from the new exported JSDoc.
6c9efa1 to
6b5c5de
Compare
#3641's docstring, repeated verbatim by #3647, argued the exemption was safe because "a browser cannot attach the signature header to a cross-origin request without a preflight the runtime does not grant". The runtime does grant it: with no configured `allowedHeaders`, `resolveNormalizedCORSPreflightPolicy` reflects whatever `Access-Control-Request-Headers` asked for, so any project whose CORS policy admits an origin advertises the signature header to it. The proxy also forwards an unverified `x-veryfront-*-jws` from a public request rather than stripping it. State the actual basis instead: the exemption skips only the browser-credential gate, authority still comes from the downstream signature verification that an attacker cannot forge, and every admitted route terminates at a handler doing that verification ahead of ApiHandlerWrapper. Same correction to the three gate comments that said the exemption is keyed on "the request being a real dispatch": no predicate can know that from a header.
The failure
A customer demo project set
to keep CSRF enforced everywhere except the agent endpoint its chat client
posts to. Deploy then failed with
The developer compared manifests across releases — four consecutive releases
carrying a nested
csrfobject have no manifest, releases before and afterreport
state=ready— and shippedcsrf: falseon a sign-in-protectedcustomer demo because it was the only setting that deployed.
Mechanism
The manifest is built by the project runtime, not the CLI. The control plane
POSTs a signed operation envelope to
/api/control-plane/runs/{runId}/executewithtarget: "task:release-asset-build"; that dispatch is the only caller ofbeginReleaseAssetManifestBuild(src/release-assets/build-executor.ts:2297).Until it lands, no manifest row exists and the state reads
missing.CsrfHandler(src/security/http/csrf/csrf-handler.ts:63) registers atpriority 5 with
patterns: [], so it runs in front ofProjectRunExecuteHandlerfor that POST(
src/server/runtime-handler/index.ts:138-159,src/routing/registry/registry.ts:86-101). The control plane is not a browser:it carries no
__Host-vf_csrfcookie and authorizes from the JWS the handlerverifies. So the gate answered the project's own build dispatch with
403 Forbidden – invalid or missing CSRF token, the run failed before themanifest existed, and the deploy surfaced 120 seconds later naming neither
CSRF nor config.
The boundary is not the nested object.
csrf: truefails identically —only an absent or
falsesetting ever built a manifest. Absent is the commoncase because the task rail dispatches to the main-branch runtime, which
resolves as preview, so the production
csrfdefault never applies there(
src/security/http/config.ts:271). That is why manifests build for everyonewho never wrote the key.
Fix
isControlPlaneSurfaceRequest(src/channels/control-plane.ts) exempts thesigned control-plane prefix from the CSRF gate — the same shape as the existing
isCspReportRequestexemption, and for the same reason: the gate expects abrowser credential the caller cannot hold. Every handler behind that prefix
authenticates its envelope through
verifyControlPlaneRequestbefore acting(runs execute/resume/cancel/stream, agents list), so the exemption removes no
authorization. The prefix is matched against
URL.pathname, which resolves dotsegments, and a path that merely starts alike
(
/api/control-plane-mirror/...) stays gated.Tests
src/release-assets/build-dispatch-security.test.tsdrives the real chain —CsrfHandlerthenProjectRunExecuteHandler, with a real signed envelope —across all four csrf shapes and asserts the release asset build executor is
reached.
Red, before the fix:
Green, after:
Plus two cases in
csrf-handler.test.ts: a run dispatch passes for everyenabled csrf shape, and a look-alike prefix is still rejected. No existing test
was changed or weakened.
Not fixed here, worth a follow-up
AuthHandlerhas the same hole. It runs at priority 0, exempts onlyisCspReportRequest, and readssecurity.auth(
src/security/http/auth.ts:156-183). A project that configures basic orbearer auth in
veryfront.config.tsshould be expected to 401 its owncontrol-plane dispatches the same way. Not changed here because it needs its
own reproduction rather than a symmetric guess.
value can still stop a manifest with the failure surfacing 120 s later as a
timeout that names no cause. What would close that: have
deployread therelease-asset build run's terminal state and error alongside the manifest
state, so
last state: missingcan say why the build never began.Summary by CodeRabbit
Bug Fixes
Tests